-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a Contributor Guide #42
base: main
Are you sure you want to change the base?
Conversation
|
||
## Environment setup and workflow | ||
- Clone the repo | ||
- Create a fresh python environemt with `.binder/requirements.txt` file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bsipocz I noticed you recently moved requirements.txt to .binder/
from tutorials/
. Can we keep it in tutorials/
and create a symlink to it in .binder/
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Symlinks notoriously won't work on windows, at least I had issues with keeping symlinks in repos before (and we only symlinked licences and contributing guides rather than actually important content)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought users wouldn't need to touch this requirements.txt file all since notebooks in this repo should always pip install
what they need in the first code cell -- I thought this file was only here for the rendering? I do see that the instructions below have some dependency on in it (i.e., assuming jupytext extension is installed). Since my workflow with notebooks is so different I don't know exactly what the user will need, so I'm fine with whatever's easiest (either telling them to install that extension separately or just pointing them to the requirements.txt file even though it isn't intended for them). Although, the other repo doesn't have a requirements.txt, so if this file gets moved there that's another factor to think about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not yet sure if this is the right repo for this document, or we should rather keep it in the other repo (after all that's where we have the template, too).
Anyway, I'll give this a more detailed review later this week.
- Create a fresh python environemt with `.binder/requirements.txt` file | ||
- Open `jupyter lab` or `jupyter notebook` (so that jupytext extension can take effect) | ||
- Then right-click on a MyST markdown notebook, choose `Open With` -> `Notebook` | ||
or `Jupytext Notebook` (more info [here](https://nasa-navo.github.io/navo-workshop/00_SETUP.html#handling-notebooks-in-myst-markdown-format)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should link more upstream than NAVO, e.g. numpy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you share the link? Last I checked numpy docs, I got confused because they were instructing to "pairing the notebook and md" but then I learned from you that that's not required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I knew it would bite me back, we need to cleanup the numpy docs first, 😅 : numpy/numpy-tutorials#186
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jaladh-singhal, this is helpful. I don't often work in a jupyter environment (I'm much more likely to convert the .md to .py and just use an editor), so I don't really have thoughts/opinions about the actual instructions here except to say that I think they're much needed since contributors are likely to be working this way.
Organizationally, we should merge this with the template @bsipocz referred to so that we have one consistent set of "contributing" documentation. That template is for new notebooks and is here: https://github.com/IPAC-SW/ipac-sp-notebooks/tree/main/template.
In terms of which repo, we had talked about having contributions go into the other repo both so that they can be worked on in private and so we can better control the content in this repo (since the rendering, etc. means there are more requirements/constraints here). But I'll defer to @bsipocz on that since she does most of the management tasks that would be affected.
In terms of the content, there's a little bit of overlap so we should make sure those parts are consistent (and this will depend on which repo we choose). Also, maybe the content in the template's README should actually go in the 'new notebook' section of this file. Then we wouldn't need that separate file. @jaladh-singhal take a look at that readme and the template and see what you think.
|
||
## Environment setup and workflow | ||
- Clone the repo | ||
- Create a fresh python environemt with `.binder/requirements.txt` file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought users wouldn't need to touch this requirements.txt file all since notebooks in this repo should always pip install
what they need in the first code cell -- I thought this file was only here for the rendering? I do see that the instructions below have some dependency on in it (i.e., assuming jupytext extension is installed). Since my workflow with notebooks is so different I don't know exactly what the user will need, so I'm fine with whatever's easiest (either telling them to install that extension separately or just pointing them to the requirements.txt file even though it isn't intended for them). Although, the other repo doesn't have a requirements.txt, so if this file gets moved there that's another factor to think about.
```python | ||
jupytext --to md:myst notebook.ipynb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
```python | |
jupytext --to md:myst notebook.ipynb | |
```sh | |
jupytext --to md:myst notebook.ipynb |
jupytext --to md:myst notebook.ipynb | ||
``` | ||
|
||
Then add it to `tutorials/` and also add its path in a relevant TOC in `index.md`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just flagging this line because there are similar instructions in the existing docs in the other repo, but the process is different in that repo. So we need to sync them up depending on which repo we chose.
Aims to fix #17 esp the bulk usage part
I've tried to draft a documentation based on my development experience on this repository so far. It's very minimal and can use a lot of changes (and maybe restructuring too?)
Marking it as draft since it needs feedback from other developers (@bsipocz, @troyraen, ...?) before adding more content